Retry at most once when ECH fails - #9664
Merged
Merged
Conversation
swankjesse
commented
Aug 13, 2026
| // From https://cs.android.com/android/platform/superproject/+/android-latest-release:external/conscrypt/platform/src/main/java/org/conscrypt/Platform.java;bpv=0 | ||
| // we can get neither, publicHostname only, or both. Conscrypt only hands us an EchConfigList | ||
| // if it is non-empty and self-consistent; BoringSSL does the real validation (version checks | ||
| // and such) when we hand the list back to it. |
Collaborator
Author
There was a problem hiding this comment.
Docs now in the EchRetry class
| } | ||
| if (!retryable) return null | ||
| // If this was an ECH retry, don't retry again. | ||
| if (echRetryPlan != null) return null |
Collaborator
Author
There was a problem hiding this comment.
Fix: previously we’d permit one retry with a EchConfigList once, and another retry with no EchConfigList. I believe we should be limited to one retry total.
| @@ -503,26 +505,15 @@ class ConnectPlan internal constructor( | |||
| ): ConnectPlan? { | |||
| if (!retryOnConnectionFailure) return null | |||
Collaborator
Author
There was a problem hiding this comment.
For follow-up, I believe we need to reorder these so we do an ECH retry even when this is false.
| * See RFC 9849, section 6.1.6. | ||
| */ | ||
| @OkHttpInternalApi | ||
| class EchRetryPlan private constructor( |
Collaborator
Author
There was a problem hiding this comment.
Renamed from EchRetryConfig
Collaborator
There was a problem hiding this comment.
It's grown on me already
| assertInvalid("1:2::3:4") | ||
| assertInvalid("10.20.30.40") | ||
| assertInvalid("ech.example.com.") | ||
| assertInvalid(".ech.example.com") |
Collaborator
Author
There was a problem hiding this comment.
Our existing hostname checks permit this
| ) | ||
| val publicServer = | ||
| server.copy( | ||
| keyManager = untrustedServerCertificates.keyManager, |
Collaborator
Author
There was a problem hiding this comment.
This one isn’t signed by our CA
swankjesse
force-pushed
the
jwilson.0813.more_retry_tests
branch
from
August 13, 2026 22:11
7f14f27 to
c970b18
Compare
yschimke
approved these changes
Aug 13, 2026
Base automatically changed from
jwilson.0813.securely_disable_ech_test
to
main
August 14, 2026 03:16
Also write some new ECH retry tests. Also enforce restrictions on public_name structure. Also rename EchRetryConfig to EchRetryPlan.
swankjesse
force-pushed
the
jwilson.0813.more_retry_tests
branch
from
August 14, 2026 03:22
c970b18 to
4c53b33
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Also write some new ECH retry tests.
Also enforce restrictions on public_name structure.
Also rename EchRetryConfig to EchRetryPlan.